Skip to content

Bugfix/broken access control on observability REST APIs (IDOR)#590

Open
sakith03 wants to merge 6 commits intowso2:mainfrom
sakith03:bugfix/broken-access-control
Open

Bugfix/broken access control on observability REST APIs (IDOR)#590
sakith03 wants to merge 6 commits intowso2:mainfrom
sakith03:bugfix/broken-access-control

Conversation

@sakith03
Copy link
Copy Markdown
Contributor

Purpose

The /icp/observability REST endpoints accepted componentId / environmentId filters and resolved runtime IDs without enforcing that the authenticated caller is authorized to access those runtimes. This could allow a user with a valid token to retrieve logs/metrics for integrations/environments they should not have access to.
Resolves unauthorized observability access (IDOR) in logs/metrics endpoints.

Goals

Enforce RBAC-based authorization for observability log/metric retrieval.
Ensure observability queries only return data for runtimes accessible to the authenticated user.
Keep behavior stable for authorized users and avoid changes that impact performance or response shape.
Approach
Kept existing JWT auth on the service (@http:ServiceConfig with jwtValidatorConfig).
Added extraction of the authenticated user context from the request Authorization header using existing auth utilities.
Filtered resolved runtime IDs using existing storage-layer access checks (storage:hasAccessToRuntime(userId, runtimeId)), and only forwarded authorized runtime IDs to the observability adapter.
No UI changes.
User stories
As a user, I can only view logs/metrics for integrations and environments I have access to.
As an admin, I can prevent cross-project/environment data leakage via observability endpoints.
Release note
Fix: Enforced authorization on ICP observability logs/metrics endpoints to prevent unauthorized access to runtime observability data.

Automation tests

Unit tests
-Code coverage information: N/A (existing project test suite; no coverage report generated in this run)
Integration tests
-Ran bal test inside Docker against a fresh MySQL test DB: 74 passing, 0 failing, 0 skipped.
Security checks
-Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
-Ran FindSecurityBugs plugin and verified report? no (not run as part of this change validation)
-Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Test environment

OS: Windows 10
Database: MySQL (Docker, mysql-db from icp_server/docker-compose.local.yml)
Backend runtime: Ballerina (inside Docker image wso2/icp_server)
Learning
Reused existing authorization patterns already present in the codebase (JWT validation via service config + auth:extractUserContextV2 and storage access checks such as hasAccessToRuntime) to keep the fix consistent and low-risk.

sakith03 and others added 4 commits April 11, 2026 00:29
Comment thread icp_server/observability_service.bal
Comment thread icp_server/observability_service.bal
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5c57f583-7baa-4ed9-9d88-6a416f408ba9

📥 Commits

Reviewing files that changed from the base of the PR and between ee30de3 and 8fd4aeb.

📒 Files selected for processing (1)
  • icp_server/observability_service.bal
🚧 Files skipped from review as they are similar to previous changes (1)
  • icp_server/observability_service.bal

📝 Walkthrough

Changes

Modified icp_server/observability_service.bal to enforce access control on observability data retrieval:

  • Added user context extraction from the incoming Authorization header to establish the request context
  • Implemented authorization filtering on both /observability/logs and /observability/metrics endpoints so that only runtime data accessible to the authenticated user is included in responses
  • Requests for runtimes outside the user's access scope return empty results through the existing filtering logic

Impact

Users can now only retrieve observability logs and metrics for runtimes they are authorized to access. The change maintains existing API response shapes and behavior for authorized users.

Walkthrough

Adds request-scoped user context extraction and runtime-level authorization to observability endpoints. The service now reads the incoming Authorization header, converts it to a UserContextV2, and uses that user identity to filter runtime IDs returned by resolveRuntimeIds. Both /observability/logs and /observability/metrics apply storage:hasAccessToRuntime per runtime ID and proceed with downstream processing using the post-filtered runtime list; requests with no accessible runtimes yield empty responses after filtering.

Changes

Cohort / File(s) Summary
Observability Service Authorization
icp_server/observability_service.bal
Added extractUserFromObservabilityRequest() to extract UserContextV2 from the Authorization header (errors when absent). Added filterRuntimeIdsForUser() to call storage:hasAccessToRuntime and filter runtime IDs. Updated /observability/logs and /observability/metrics flows to extract user context and filter runtime IDs by user access before applying component/environment filters and invoking adapters. No exported/public signatures changed.

Sequence Diagram

sequenceDiagram
    participant Client
    participant ObservabilityService
    participant AuthModule
    participant StorageService
    participant Adapter

    Client->>ObservabilityService: HTTP request (with Authorization header)
    ObservabilityService->>AuthModule: extractUserContextV2(Authorization)
    AuthModule-->>ObservabilityService: UserContextV2
    ObservabilityService->>ObservabilityService: resolveRuntimeIds()
    ObservabilityService-->>ObservabilityService: runtimeIdList
    loop per runtimeId
        ObservabilityService->>StorageService: hasAccessToRuntime(userId, runtimeId)
        StorageService-->>ObservabilityService: accessGranted / accessDenied
    end
    ObservabilityService->>ObservabilityService: filterRuntimeIdsForUser() -> filtered runtimeIdList
    ObservabilityService->>ObservabilityService: apply component/environment filters
    ObservabilityService->>Adapter: invoke with filtered runtime IDs
    Adapter-->>ObservabilityService: adapter response
    ObservabilityService-->>Client: HTTP response
Loading

Estimated code review effort

High | ~45 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing a broken access control vulnerability (IDOR) in observability REST APIs.
Description check ✅ Passed The description covers most required sections including Purpose, Goals, Approach, User stories, Release note, and Security checks, with adequate detail for understanding the change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@wso2-engineering wso2-engineering Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
  • Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

  • Review all AI-generated comments for accuracy and relevance.
  • Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Comment Accepted (Y/N) Reason
#### Log Improvement Suggestion No: 1
#### Log Improvement Suggestion No: 2

sakith03 and others added 2 commits April 19, 2026 20:42
Co-authored-by: wso2-engineering[bot] <229087779+wso2-engineering[bot]@users.noreply.github.com>
Co-authored-by: wso2-engineering[bot] <229087779+wso2-engineering[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
icp_server/observability_service.bal (2)

176-184: Extract the user context before resolveRuntimeIds to fail fast.

resolveRuntimeIds already hits the DB (potentially multiple storage:getRuntimes calls under a component×environment cross-product). If the Authorization header is malformed or extractUserContextV2 fails, that work is wasted and the same logic is duplicated across the logs and metrics handlers.

Moving the user-context extraction above resolveRuntimeIds in both resource functions also narrows the auth-vs-data ordering: every request that reaches the storage layer will have already produced a validated userId, which simplifies future auditing/logging and reduces the DoS surface for tokens that decode but are semantically invalid.

♻️ Suggested ordering (logs handler)
     resource function post logs(http:Request request, types:ICPLogEntryRequest logRequest) returns types:LogEntriesResponse|http:Response|error {
         log:printDebug("Received log request: " + logRequest.toString());
 
+        types:UserContextV2 userContext = check extractUserFromObservabilityRequest(request);
+
         // Transform ICPLogEntryRequest to LogEntryRequest by resolving component/environment filters to runtime IDs
         string[] runtimeIdList = check resolveRuntimeIds({
                                                              componentId: logRequest.componentId,
                                                              componentIdList: logRequest.componentIdList,
                                                              environmentId: logRequest.environmentId,
                                                              environmentList: logRequest.environmentList
                                                          });
-
-        types:UserContextV2 userContext = check extractUserFromObservabilityRequest(request);
         runtimeIdList = check filterRuntimeIdsForUser(userContext.userId, runtimeIdList);

Apply the analogous change at lines 262-270 for the metrics handler.

Also applies to: 262-270

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@icp_server/observability_service.bal` around lines 176 - 184, Move the user
extraction to run before DB work: call
extractUserFromObservabilityRequest(request) and assign types:UserContextV2
userContext before invoking resolveRuntimeIds so malformed/invalid auth fails
fast; then pass userContext.userId into filterRuntimeIdsForUser as you already
do. Apply the same reorder in the metrics handler (the analogous block that
calls resolveRuntimeIds and filterRuntimeIdsForUser) so extraction of
userContext always happens prior to resolveRuntimeIds.

134-147: N+1 database roundtrips per request under filterRuntimeIdsForUser.

storage:hasAccessToRuntime issues ~3 DB queries per runtime (runtime lookup + integration-access view + optional environment restriction). Called sequentially in this loop, a query that resolved M runtimes will incur ~3M synchronous roundtrips before the adapter is even called, which can dominate request latency for broad component/environment filters and contradicts the "keep performance stable" goal stated in the PR objectives.

Consider a single batch authorization check that scopes directly in SQL, e.g. an IN (?, ?, ...) join against v_user_integration_access (plus environment restriction) that returns the subset of accessible runtime IDs in one roundtrip. This also avoids partial-failure states where some checks succeed and a later one errors out, leaving the caller unsure whether partial filtering occurred.

♻️ Sketch (pseudo-API)
-isolated function filterRuntimeIdsForUser(string userId, string[] runtimeIds) returns string[]|error {
-    string[] filtered = [];
-    foreach string rid in runtimeIds {
-        boolean|error allowed = storage:hasAccessToRuntime(userId, rid);
-        if allowed is error {
-            return allowed;
-        }
-        if allowed {
-            filtered.push(rid);
-        }
-    }
-    return filtered;
-}
+isolated function filterRuntimeIdsForUser(string userId, string[] runtimeIds) returns string[]|error {
+    if runtimeIds.length() == 0 {
+        return runtimeIds;
+    }
+    // Single DB roundtrip returning only the subset of runtimeIds the user can access.
+    return storage:filterAccessibleRuntimes(userId, runtimeIds);
+}

Implementation on the storage side would be roughly:

SELECT r.runtime_uuid
FROM runtimes r
JOIN v_user_integration_access v
  ON v.integration_uuid = r.integration_uuid AND v.user_uuid = ?
LEFT JOIN <env_restriction_view> e
  ON e.user_uuid = v.user_uuid AND e.integration_uuid = r.integration_uuid
WHERE r.runtime_uuid IN (?, ?, ...)
  AND (r.env_uuid IS NULL OR e.env_uuid IS NULL OR e.env_uuid = r.env_uuid);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@icp_server/observability_service.bal` around lines 134 - 147, The current
filterRuntimeIdsForUser loops and calls storage:hasAccessToRuntime per runtime
causing N+1 DB roundtrips; replace this with a single batch storage call that
returns the subset of accessible runtime IDs in one query (eg. add/use a storage
method like storage:getAccessibleRuntimeIds(userId, runtimeIds) or
storage:filterAccessibleRuntimes(userId, runtimeIds) which implements an IN(...)
join against v_user_integration_access with the environment restriction), then
have filterRuntimeIdsForUser call that single method, propagate any error
directly, and return the resulting string[] to avoid partial-success states and
make authorization one roundtrip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@icp_server/observability_service.bal`:
- Around line 176-184: Move the user extraction to run before DB work: call
extractUserFromObservabilityRequest(request) and assign types:UserContextV2
userContext before invoking resolveRuntimeIds so malformed/invalid auth fails
fast; then pass userContext.userId into filterRuntimeIdsForUser as you already
do. Apply the same reorder in the metrics handler (the analogous block that
calls resolveRuntimeIds and filterRuntimeIdsForUser) so extraction of
userContext always happens prior to resolveRuntimeIds.
- Around line 134-147: The current filterRuntimeIdsForUser loops and calls
storage:hasAccessToRuntime per runtime causing N+1 DB roundtrips; replace this
with a single batch storage call that returns the subset of accessible runtime
IDs in one query (eg. add/use a storage method like
storage:getAccessibleRuntimeIds(userId, runtimeIds) or
storage:filterAccessibleRuntimes(userId, runtimeIds) which implements an IN(...)
join against v_user_integration_access with the environment restriction), then
have filterRuntimeIdsForUser call that single method, propagate any error
directly, and return the resulting string[] to avoid partial-success states and
make authorization one roundtrip.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 177c2044-fdf7-4250-990d-5988caba42a7

📥 Commits

Reviewing files that changed from the base of the PR and between bef05a9 and ee30de3.

📒 Files selected for processing (1)
  • icp_server/observability_service.bal

@anuruddhal
Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants